SF-3913 Pause sending of ops to ShareDB if offline - #4088
pmachapman wants to merge 1 commit into
Conversation
pmachapman
added
will require testing
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4088 +/- ##
=======================================
Coverage 80.95% 80.95%
=======================================
Files 664 664
Lines 42153 42165 +12
Branches 6944 6919 -25
=======================================
+ Hits 34126 34136 +10
- Misses 6864 6881 +17
+ Partials 1163 1148 -15 ☔ View full report in Codecov by Harness. |
RaymondLuong3
left a comment
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1232 at r1 (raw file):
if (!this.onlineStatusService.isOnline) { textDoc.adapter.pause(); }
Do you think these lines are necessary? There may be a small window when the component initializes and the app is offline but onlineStatus$ has not emitted yet, but that window is so small. Is that what these lines are for?
Code quote:
if (!this.onlineStatusService.isOnline) {
textDoc.adapter.pause();
}
pmachapman
left a comment
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1232 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Do you think these lines are necessary? There may be a small window when the component initializes and the app is offline but
onlineStatus$has not emitted yet, but that window is so small. Is that what these lines are for?
Yes - I found it was necessary to do this in my testing.
RaymondLuong3
left a comment
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
RaymondLuong3
added
ready to test
and removed
will require testing
97f88c6 to
2845b49
Compare
2 participants
Problem
I isolated the root of this issue to client 1 thinking it was offline, and so triggering the logic in
TextViewModel.fixSegment()to not create blanks (see SF-2272 and SF-2722), but the client was not completely offline - it was just a flaky connection. Because an op was sent to ShareDB that cleared a segment, but did not create a blank in its place, another listening client creates the blank, and sends it to ShareDB. When client 1 comes back online, it can under certain circumstances become confused, as it will attempt to create a blank. This will often resolve correctly, but if other edits took place, duplication of segments can occur sometimes (but not all the time). The duplication results from ShareDB's resolution of any incoming and outgoing ops on the segment, and its attempt to reconcile them.Resolution
To resolve this issue, I pause any updates being sent to ShareDB for the text document if Scripture Forge believes itself to be offline. Ops are resumed when Scripture Forge next is online. This means that the omission of blank ops by
TextViewModel.fixSegment()is not replicated to ShareDB (it is not supposed to be as this is offline-only functionality to ensure offline edits do not corrupt valid incoming edits from other users - see SF-2272)Other Solutions
As the root of this issue is blank ops, and the view model's handling of them, the only other solution I could come to is the currently mothballed SF-3437 / #3339.
This change is